Form read write split by aolivier23 · Pull Request #442 · Framework-R-D/phlex

aolivier23 · 2026-03-23T15:41:27Z

This PR splits form_interface into two components: form_reader_interface and form_writer_interface. This split saves FORM time by avoiding lock contention between read and write processes and prepares for RNTuple support that does not have members for an RNTupleWriter when only reading from a container.

All of the layers that power the former form_interface are split in this way. A handful of functions shared between reading and writing have been made free functions in form::experimental::detail.

form_interface is now split between form_reader_interface and form_writer_interface for example.

been replaced by PersistenceReader and PersistenceWriter.

in persistence directory.

Removed a few files that were restored in the merge with main accidentally.

aolivier23 · 2026-03-23T15:43:13Z

@phlexbot format

github-actions · 2026-03-23T15:44:34Z

Format Fixes Applied

✅ clang-format fixes pushed (commit d586ade)
✅ cmake-format fixes pushed (commit 0e84f5f)
✅ ruff fixes pushed (commit ee7bae1)
✅ YAML formatter fixes pushed (commit 23e120b)

⚠️ Note: Some issues may require manual review.

greenc-FNAL · 2026-03-23T15:46:03Z

Review the full CodeQL report for details.

codecov · 2026-03-23T15:53:43Z

Codecov Report

❌ Patch coverage is 83.49515% with 68 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
form/root_storage/root_tbranch_read_container.cpp	76.78%	8 Missing and 5 partials ⚠️
form/root_storage/root_tbranch_write_container.cpp	75.47%	9 Missing and 4 partials ⚠️
form/root_storage/root_ttree_write_container.cpp	50.00%	6 Missing and 6 partials ⚠️
form/storage/storage_reader.cpp	82.60%	4 Missing and 4 partials ⚠️
form/storage/storage_writer.cpp	84.78%	4 Missing and 3 partials ⚠️
form/util/factories.hpp	77.77%	0 Missing and 4 partials ⚠️
form/persistence/persistence_reader.cpp	84.21%	2 Missing and 1 partial ⚠️
form/root_storage/root_ttree_read_container.cpp	70.00%	2 Missing and 1 partial ⚠️
form/form/form_reader.cpp	83.33%	1 Missing and 1 partial ⚠️
form/root_storage/demangle_name.cpp	71.42%	1 Missing and 1 partial ⚠️
... and 1 more

@@            Coverage Diff             @@
##             main     #442      +/-   ##
==========================================
+ Coverage   85.00%   85.33%   +0.33%     
==========================================
  Files         127      145      +18     
  Lines        3341     3431      +90     
  Branches      574      583       +9     
==========================================
+ Hits         2840     2928      +88     
+ Misses        306      303       -3     
- Partials      195      200       +5

Flag	Coverage Δ
unittests	`85.33% <83.49%> (+0.33%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
form/form/form_reader.hpp	`100.00% <100.00%> (ø)`
form/form/form_writer.cpp	`77.77% <100.00%> (ø)`
form/form/form_writer.hpp	`100.00% <100.00%> (ø)`
form/form_module.cpp	`89.36% <100.00%> (+0.23%)`	⬆️
form/persistence/ipersistence_reader.hpp	`100.00% <100.00%> (ø)`
form/persistence/ipersistence_writer.hpp	`100.00% <100.00%> (ø)`
form/persistence/persistence_reader.hpp	`100.00% <100.00%> (ø)`
form/persistence/persistence_writer.cpp	`100.00% <100.00%> (ø)`
form/persistence/persistence_writer.hpp	`100.00% <100.00%> (ø)`
form/root_storage/root_tbranch_read_container.hpp	`100.00% <100.00%> (ø)`
... and 26 more

... and 3 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3bda620...c47a8c6. Read the comment docs.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

uses aren't available yet.

…/phlex into form_read_write_split

code coverage test.

gemmeren · 2026-03-23T20:45:35Z

form/form/form_reader.cpp

+    }
+
+    m_pers = form::detail::experimental::createPersistenceReader();
+    m_pers->configureOutputItems(output_config);


For Reader?

I think you're correct about this. Will review and probably change. Thanks.

I think this needs a rename but can't be eliminated without rewriting more of PersistenceReader. We use m_output_items to figure out what file and technology to use when making a Token to get row ID.

gemmeren · 2026-03-23T20:46:14Z

form/form/form_reader.cpp

+
+  form_reader_interface::form_reader_interface(config::output_item_config const& output_config,
+                                               config::tech_setting_config const& tech_config) :
+    m_pers(nullptr)


suggest rename

How about output_item_config -> io_item_config? container_config? I think we're going to need it in both the reader and the writer.

The location information on data to read should not depend on configuration.

Other than the (global) input file-names, a reading job does not need to tell FORM where and what objects are stored, FORM should find them. Whereas on output, configuration does determine which outputs are written where.

gemmeren · 2026-03-23T20:48:24Z

form/form/form_writer.cpp

-                                 config::tech_setting_config const& tech_config) :
+  form_writer_interface::form_writer_interface(config::output_item_config const& output_config,
+                                               config::tech_setting_config const& tech_config) :
    m_pers(nullptr)


Suggest rename

gemmeren · 2026-03-23T20:49:49Z

form/form/product_with_name.hpp

Why did this change?

Will investigate. I agree that it shouldn't change. Thank you for catching this.

gemmeren · 2026-03-23T20:50:49Z

form/persistence/ipersistence_reader.hpp

+    virtual void configureTechSettings(
+      form::experimental::config::tech_setting_config const& tech_config_settings) = 0;
+
+    virtual void configureOutputItems(


Agreed, I expect that I can remove everything dealing with output config from the reader-flavor components. I'll report back if it doesn't work for some reason.

form/root_storage/root_tbranch_read_container.cpp

gemmeren · 2026-03-23T20:54:13Z

form/root_storage/root_tbranch_read_container.cpp

+  }
+}
+
+void ROOT_TBranch_Read_ContainerImp::setFile(std::shared_ptr<IStorage_File> file)


for reading?

This one I think has some reasonable future applications. We might want to read multiple input files in the same job in the future like the old framework does. It might well be a no-op right now though.

Why do you think we should remove it?

Once you have a container (on read), wouldn't it know its file?

gemmeren · 2026-03-23T20:54:40Z

form/root_storage/root_tbranch_read_container.cpp

+  return;
+}
+
+void ROOT_TBranch_Read_ContainerImp::setParent(std::shared_ptr<IStorage_Read_Container> parent)


not sure whether we need this for read.

Will investigate, thanks.

form/root_storage/root_ttree_read_container.cpp

gemmeren · 2026-03-23T20:56:33Z

form/storage/storage_associative_read_container.cpp

Not sure that we would split associative... Do we need them on read and write?

reading.

aolivier23 · 2026-03-23T23:07:48Z

@phlexbot format

github-actions · 2026-03-23T23:08:42Z

Format Fixes Applied

✅ clang-format fixes pushed (commit c47a8c6)

⚠️ Note: Some issues may require manual review.

aolivier23 added 9 commits February 27, 2026 13:33

Split all FORM components into reading and writing components.

2ffcd6b

form_interface is now split between form_reader_interface and form_writer_interface for example.

Removing features from old prototype branch.

0be8f74

Removed old unified Persistence class that is no longer needed. It has

75fdbc5

been replaced by PersistenceReader and PersistenceWriter.

Removed old unified Storage class that is no longer used.

471b535

Renamed storage_util.hpp to storage_utils.hpp to match naming convention

b7ddde1

in persistence directory.

Merge branch 'main' into form_read_write_split

c0071a7

Got latest FORM Storage tests working with the read/write split.

e464bda

Removed a few files that were restored in the merge with main accidentally.

Merge branch 'main' into form_read_write_split

3e7360e

Merge branch 'main' into form_read_write_split

aac5c6a

aolivier23 requested review from gemmeren and wwuoneway March 23, 2026 15:41

github-actions bot added 4 commits March 23, 2026 15:43

Apply YAML formatter fixes

23e120b

Apply cmake-format fixes

0e84f5f

Apply clang-format fixes

d586ade

Apply ruff fixes

ee7bae1

aolivier23 added 4 commits March 23, 2026 11:00

Removed accidental RNTuple inclusion from factories. The classes it

a28c500

uses aren't available yet.

Merge branch 'main' into form_read_write_split

0c07e0a

Merge branch 'form_read_write_split' of https://github.com/aolivier23…

4346c13

…/phlex into form_read_write_split

Removed accidental mention of RNTuple components that was setting off

7c9742a

code coverage test.

gemmeren requested changes Mar 23, 2026

View reviewed changes

aolivier23 added 2 commits March 23, 2026 17:00

Removed attribute for TBranch read container that is not relevant to

82aa92f

reading.

Removed Write command in TTree read container that's no longer needed.

0153e49

Apply clang-format fixes

c47a8c6

Conversation

aolivier23 commented Mar 23, 2026

Uh oh!

aolivier23 commented Mar 23, 2026

Uh oh!

github-actions bot commented Mar 23, 2026

Format Fixes Applied

Uh oh!

greenc-FNAL commented Mar 23, 2026

Uh oh!

codecov bot commented Mar 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aolivier23 commented Mar 23, 2026

Uh oh!

github-actions bot commented Mar 23, 2026

Format Fixes Applied

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Mar 23, 2026 •

edited

Loading